Skip to content

Refactor deconv.py#35

Open
daharoni wants to merge 4 commits intomainfrom
refactor-deconv
Open

Refactor deconv.py#35
daharoni wants to merge 4 commits intomainfrom
refactor-deconv

Conversation

@daharoni
Copy link
Member

@daharoni daharoni commented Dec 13, 2025

Follows #34

Breaking out deconv.py into:

  • config.py: Holds a pydantic model for input params into deconv. It also freezes the data in the config
  • utils.py: Holds small lightweight methods
  • solver.py: implements the CVXPYSolver and OSQPSolver using base class DeconvSolver
  • deconv.py: everything else

Testing and comparisions to old deconv.py

I ran some comparison sweeps across the old and new deconv code

solve

  • solve looks to perfectly replicate the old code. The refactor didn’t change the core solver output.

difference in svals_len

  • We are seeing a difference in some solve_thres behavior.
  • The vast majority of solve_thres failures show the exact same debug signature:
  • old_svals_len=999, new_svals_len=1
  • That’s a bug in the old code I think: min_rel_scl filtering shrinks scals/bs but not svals/yfvals in the old code, so objective evaluation zips mismatched lists and effectively evaluates the “wrong” candidate(s).
  • To verify, I implemented this bug in the new code and that results in identical results. This is left in currently if you enable legacy_bug in deconv.py.

@daharoni
Copy link
Member Author

I just ran through a parameter sweep using both the old deconv.py code and this refactored code. These look to be functionally identical except the refactor fixes one bug in the old code which causes the results to diverge from old code. The PR description has more on that.

@daharoni daharoni mentioned this pull request Dec 14, 2025
@daharoni daharoni marked this pull request as ready for review December 14, 2025 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant